Skip to content

fix(run): Printing a new line to avoid overwriting error code after \r - #17373

Open
Suryansh-Dey wants to merge 2 commits into
rust-lang:masterfrom
Suryansh-Dey:newline
Open

fix(run): Printing a new line to avoid overwriting error code after \r#17373
Suryansh-Dey wants to merge 2 commits into
rust-lang:masterfrom
Suryansh-Dey:newline

Conversation

@Suryansh-Dey

@Suryansh-Dey Suryansh-Dey commented Aug 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What does this PR try to resolve?

Closes #17343
Changes:
Added newline using writeln!(gctx.shell().err());
Reason:
Could have used format!("\n{err}") but if there's an in-progress status line, gctx.shell().err() would erase it then print a newline. If you want I can switch to format option but this feels practically safer.

How to test and review this PR?

  • Run cargo test -p cargo --test testsuite -- run::exit_code run::exit_code_verbose And both the tests passes.
  • Make a program in windows
fn main() {
    print!("hewwo\r");
    std::process::exit(1);
}

then run cargo run. It prints error message just below.
image
(Compiling for windows almost killed my laptop)

Before (current cargo on windows)

image

@rustbot rustbot added A-cli Area: Command-line interface, option parsing, etc. Command-run S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 18, 2026
@rustbot

rustbot commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

r? @epage

rustbot has assigned @epage.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: @epage, @weihanglo
  • @epage, @weihanglo expanded to epage, weihanglo
  • Random selection from epage, weihanglo

epage
epage reviewed Aug 18, 2026
View reviewed changes
Comment thread src/bin/cargo/commands/run.rs Outdated
@Suryansh-Dey

Suryansh-Dey commented Aug 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Thank you, done!
Update: I can't understand how changing a comment is making an old test fail for a file I never touched!
I ran cargo clippy --workspace --all-targets --no-deps --keep-going locally and it passed!

@rustbot

This comment has been minimized.

Sign in to view
@rustbot rustbot added the A-testing-cargo-itself Area: cargo's tests label Aug 20, 2026
@Suryansh-Dey

Suryansh-Dey commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Fixed that clippy error (though was not part of this PR) but now I think this error is due to some recent updates and not related to my changes at all.

Suryansh-Dey
Suryansh-Dey commented Aug 21, 2026
View reviewed changes
Comment thread crates/cargo-test-support/src/compare.rs

@Suryansh-Dey Suryansh-Dey Aug 21, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To get rid of clippy error from CI checks

View changes since the review

Suryansh-Dey
Suryansh-Dey commented Aug 21, 2026
View reviewed changes
Comment thread crates/cargo-test-support/Cargo.toml
@rustbot

rustbot commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@Suryansh-Dey
Suryansh-Dey requested a review from epage August 21, 2026 05:01
@0xPoe 0xPoe changed the title Fix(error): Printing a new line to avoid overwriting error code after \r fix: Printing a new line to avoid overwriting error code after \r Aug 21, 2026
0xPoe
0xPoe reviewed Aug 21, 2026
View reviewed changes

@0xPoe 0xPoe left a comment •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

View changes since this review

Comment thread tests/testsuite/run.rs
[COMPILING] foo v0.0.1 ([ROOT]/foo)
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
[RUNNING] `target/debug/foo[EXE]`

@0xPoe 0xPoe Aug 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change catches the new line we added, but it’s difficult to understand the exact problem we’re trying to solve. Could we add the case from the issue as a regression test? We could first capture the previous behavior in one commit and then fix it in a follow-up commit. That would make it much easier to understand the issue we’re addressing on the Windows platform.

For the commit message format, we usually include the scope in parentheses. In Cargo, this typically refers to the subcommands, and we generally don’t capitalize it. Therefore, the most appropriate format for this change may be fix(run):.

@Suryansh-Dey Suryansh-Dey Aug 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for guiding me.
I have added a test which had overwriting stdout as expected output in first commit.
As you can see tests passes on windows after first commit

Image And in next commit the expected output is printing a \n before stderr.

@0xPoe 0xPoe changed the title fix: Printing a new line to avoid overwriting error code after \r fix(run): Printing a new line to avoid overwriting error code after \r Aug 21, 2026
@Suryansh-Dey
Suryansh-Dey requested a review from 0xPoe August 21, 2026 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@epage epage Awaiting requested review from epage
@0xPoe 0xPoe Awaiting requested review from 0xPoe

At least 1 approving review is required to merge this pull request.

Assignees

@epage epage

Labels

A-cli Area: Command-line interface, option parsing, etc. A-testing-cargo-itself Area: cargo's tests Command-run S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

cargo run "process didn't exit successfully" overwrites last line of a program that uses carriage return

4 participants

@Suryansh-Dey @rustbot @epage @0xPoe